buffer: add Buffer compare by offset #5880

jasnell · 2016-03-23T21:30:42Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

Adds additional offset, length and thisOffset arguments
to Buffer.prototype.compare to allow comparison of sub-ranges
of two Buffers without requiring Buffer.prototype.slice()

Fixes: #521
/cc @trevnorris @rootslab @jorangreef

mscdex · 2016-03-24T06:01:24Z

doc/api/buffer.markdown

@@ -670,9 +670,15 @@ console.log(buf.toString('ascii'));
  // Prints: Node.js
 ```

-### buf.compare(otherBuffer)
+### buf.compare(otherBuffer[, offset[, length[, thisOffset]]])


Perhaps the order should be tweaked a bit here and have length and thisOffset swapped to match more closely with buffer.copy() which also deals with two buffers?

I also think offset might be better renamed to something like otherOffset.

Possibly follow suit of copy() since it follows similar parameters.

jasnell · 2016-03-24T16:58:37Z

@trevnorris @mscdex ... updated! PTAL

The signature has been changed to: buf.compare(other[, otherStart[, thisStart[, otherEnd[, thisEnd]]]])

mscdex · 2016-03-24T17:13:15Z

Couldn't we just have one end value, thisEnd, or just keep length instead of the end parameters? If not, I would prefer seeing buf.compare(other[, otherStart[, otherEnd[, thisStart[, thisEnd]]]]).

jasnell · 2016-03-24T17:24:04Z

Personally I'd prefer to keep length. I changed it to this to align with the signature for buf.copy()

jasnell · 2016-03-24T17:31:14Z

That said, having it as just length limits you to always comparing equal length ranges which is not always desirable.

jasnell · 2016-03-24T17:47:40Z

@mscdex ... updated ... I swapped the order of the arguments but kept separate otherEnd and thisEnd.

The signature has been changed to: buf.compare(other[, otherStart[, otherEnd[, thisStart[, thisEnd]]]])

jasnell · 2016-03-24T18:01:23Z

Currently, thisEnd defaults to this.byteLength, however, we could make it so that if thisEnd is not specified, it defaults to Math.min(this.byteLength, thisStart + (otherEnd - otherStart)))

jasnell · 2016-03-24T18:11:25Z

doing so causes surprising behavior tho:

const a = Buffer.from([1,2,3,4,5,6,7,8,9]);
const b = Buffer.from([5,6,7,8,9,1,2,3,4]);
console.log(a.compare(b, 5, 9));
  // Prints 0

mscdex · 2016-03-24T18:16:13Z

I don't think it's that much of a surprising behavior considering that the default for sourceStart for .copy() is also 0.

jasnell · 2016-03-24T18:50:57Z

the surprising behavior is that the subset of b is not compared to the full content of a. If I did Buffer.compare(a, b.slice(5, 9)) I would get a different result.

trevnorris · 2016-03-25T10:36:03Z

doc/api/buffer.markdown

+  default = `otherBuffer.byteLength`.
+* `thisStart` {Integer} The offset within `buf` at which to begin comparison.
+  Ignored when `otherStart` is undefined. default = `0`
+* `thisEnd` {Integer} The offset within `buf` at which to end comparison.


Can we use source/target to mirror copy()? Think it will have less mental impact.

we could but source/target make a lot more sense in copy than they do in compare.

trevnorris · 2016-03-25T10:40:15Z

@jasnell One documentation comment, but implementation LGTM.

jasnell · 2016-03-26T04:14:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2069/

jasnell · 2016-03-26T04:15:25Z

@mscdex ... any further thoughts on this one?

mscdex · 2016-03-26T07:45:52Z

benchmark/buffers/buffer-compare-offset.js

+
+  switch (conf.method) {
+    case 'slice':
+      bench.start();


What about moving the bench.start()/bench.end() to be closer to the actual loop in the respective functions?

mscdex · 2016-03-26T08:06:52Z

@jasnell Just a few more comments/questions.

jasnell · 2016-03-27T20:49:34Z

@trevnorris @mscdex ... updated and rebased. PTAL!

jasnell · 2016-04-07T22:35:48Z

@trevnorris ... PTAL!

trevnorris · 2016-04-08T19:43:42Z

src/node_buffer.cc

+  else if (source_start >= source_end)
+    return args.GetReturnValue().Set(-1);
+  else if (target_start >= target_end)
+    return args.GetReturnValue().Set(1);


These should have been handled in JS. Instead can they all be turned into CHECKs? e.g.

CHECK(target_start >= target_end && source_start >= source_end); CHECK_GE(source_start >= source_end); CHECK_GE(target_start >= target_end);

Since if one of these is hit then it would indicate a bug on the JS side.

trevnorris · 2016-04-08T19:44:27Z

One comment, but LGTM otherwise.

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: nodejs#521

jasnell · 2016-04-09T00:30:35Z

@trevnorris ... updated! squashed the commits. New CI: https://ci.nodejs.org/job/node-test-pull-request/2227/

trevnorris · 2016-04-09T02:17:56Z

If CI is happy LGTM

jasnell · 2016-04-09T03:15:43Z

Failures in CI are unrelated.

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: #521 PR-URL: #5880 Reviewed-By: Trevor Norris <[email protected]>

jasnell · 2016-04-09T03:18:11Z

Landed in a246689. Thank you!

jorangreef · 2016-04-09T09:10:22Z

Thanks @jasnell and @trevnorris.

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: #521 PR-URL: #5880 Reviewed-By: Trevor Norris <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: #521 PR-URL: #5880 Reviewed-By: Trevor Norris <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behavior was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) nodejs#5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) nodejs#6279 * update ESLint to 2.7.0 (silverwind) nodejs#6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) nodejs#6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) nodejs#6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) nodejs#6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) nodejs#6090 src: * add SIGINFO to supported signals (James Reggio) nodejs#6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) nodejs#6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) nodejs#6069 PR-URL: nodejs#6322

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: #521 PR-URL: #5880 Reviewed-By: Trevor Norris <[email protected]>

Buffer: * Buffer.prototype.compare can now compare sub-ranges of two Buffers (James M Snell) #5880 deps: * update to http-parser 2.7.0 (Fedor Indutny) #6279 * update ESLint to 2.7.0 (silverwind) #6132 net: * adds support for passing DNS lookup hints to createConnection() (Colin Ihrig) #6000 node: * Make the builtin libraries available for the --eval and --print CLI options (Anna Henningsen) #6207 npm: * upgrade npm to 3.8.6 (Kat Marchán) #6153 repl: * Pressing enter in the repl will repeat the last command by default if no input has been received. This behaviour was in node previously and was not removed intentionally. (Rich Trott) #6090 src: * add SIGINFO to supported signals (James Reggio) #6093 streams: * Fix a regression that caused by net streams requesting multiple chunks synchronously when combined with cork/uncork (Matteo Collina) #6164 zlib: * The flushing flag is now configurable allowing for decompression of partial data (Anna Henningsen) #6069 PR-URL: #6322

jasnell added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 23, 2016

mscdex reviewed Mar 24, 2016
View reviewed changes

trevnorris reviewed Mar 25, 2016
View reviewed changes

jasnell force-pushed the buffer-compare-offset branch from 904cdf7 to 11f429f Compare March 26, 2016 04:07

mscdex reviewed Mar 26, 2016
View reviewed changes

jasnell force-pushed the buffer-compare-offset branch from 11f429f to cd114a2 Compare March 27, 2016 20:49

trevnorris reviewed Apr 8, 2016
View reviewed changes

buffer: add Buffer.prototype.compare by offset

1e6a4c4

Adds additional `targetStart`, `targetEnd`, `sourceStart, and `sourceEnd` arguments to `Buffer.prototype.compare` to allow comparison of sub-ranges of two Buffers without requiring Buffer.prototype.slice() Fixes: nodejs#521

jasnell force-pushed the buffer-compare-offset branch from 168f7ad to 1e6a4c4 Compare April 9, 2016 00:28

jasnell closed this Apr 9, 2016

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: add Buffer compare by offset #5880

buffer: add Buffer compare by offset #5880

jasnell commented Mar 23, 2016

mscdex Mar 24, 2016

trevnorris Mar 24, 2016

jasnell commented Mar 24, 2016

mscdex commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

mscdex commented Mar 24, 2016

jasnell commented Mar 24, 2016

trevnorris Mar 25, 2016

jasnell Mar 25, 2016

trevnorris commented Mar 25, 2016

jasnell commented Mar 26, 2016

jasnell commented Mar 26, 2016

mscdex Mar 26, 2016

mscdex commented Mar 26, 2016

jasnell commented Mar 27, 2016

jasnell commented Apr 7, 2016

trevnorris Apr 8, 2016

trevnorris commented Apr 8, 2016

jasnell commented Apr 9, 2016

trevnorris commented Apr 9, 2016

jasnell commented Apr 9, 2016

jasnell commented Apr 9, 2016

jorangreef commented Apr 9, 2016

buffer: add Buffer compare by offset #5880

buffer: add Buffer compare by offset #5880

Conversation

jasnell commented Mar 23, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

mscdex Mar 24, 2016

Choose a reason for hiding this comment

trevnorris Mar 24, 2016

Choose a reason for hiding this comment

jasnell commented Mar 24, 2016

mscdex commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

jasnell commented Mar 24, 2016

mscdex commented Mar 24, 2016

jasnell commented Mar 24, 2016

trevnorris Mar 25, 2016

Choose a reason for hiding this comment

jasnell Mar 25, 2016

Choose a reason for hiding this comment

trevnorris commented Mar 25, 2016

jasnell commented Mar 26, 2016

jasnell commented Mar 26, 2016

mscdex Mar 26, 2016

Choose a reason for hiding this comment

mscdex commented Mar 26, 2016

jasnell commented Mar 27, 2016

jasnell commented Apr 7, 2016

trevnorris Apr 8, 2016

Choose a reason for hiding this comment

trevnorris commented Apr 8, 2016

jasnell commented Apr 9, 2016

trevnorris commented Apr 9, 2016

jasnell commented Apr 9, 2016

jasnell commented Apr 9, 2016

jorangreef commented Apr 9, 2016